Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Schema.infer method #311

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

dtao
Copy link
Contributor

@dtao dtao commented Nov 7, 2017

This introduces the class method Schema.infer, to infer a Schema from
concrete data. This will be useful for converting existing known-good
data (e.g. API responses) into enforceable schemas.

This introduces the class method Schema.infer, to infer a Schema from
concrete data. This will be useful for converting existing known-good
data (e.g. API responses) into enforceable schemas.
@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+0.06%) to 95.379% when pulling 786186f on dtao:dtao/add-schema-infer-method into 95489bd on alecthomas:master.

@dtao
Copy link
Contributor Author

dtao commented Nov 7, 2017

I have written code like this for my own projects and found it useful. I figured it might be nice to add it to the library itself.

Forgive me if this functionality already exists. I couldn't find it!

@svisser
Copy link
Collaborator

svisser commented Nov 8, 2017

@dtao aside from the possible discussion of whether this logic should be part of this library, I wonder whether it should take multiple samples to infer the schema?

Because it may be that a schema "should be" a list of multiple values (e.g., Optional("gender"): In(["male", "female", "other"]) rather than Optional("gender"): "male").

@tusharmakkar08
Copy link
Collaborator

I guess this functionality can be added to the library. Also, I agree with the point @svisser made.

@dtao Can we have Schema.infer taking multiple values?

@dtao
Copy link
Contributor Author

dtao commented Nov 12, 2017

I'm open to changing things, but from @svisser's comment I think we might not be on the same page re: what this method does (or should do). This is certainly at least partially my fault for not being more thorough in the PR description (and perhaps in the docstring as well).

The way the method works in the current implementation isn't that "male" gets converted to "male"; rather, it get converted to str. The use case I have in mind is that you have a known-good object (what is sometimes called "golden data")—perhaps already available in a handy JSON file—and you want to create a schema to validate objects that look like that object, i.e. have the same keys, with values of the same types.

So:

s = Schema.infer({
    'int': 42,
    'str': 'foo',
    'bool': True
})

s({'int': 27, 'str': 'bar', 'bool': False})  # valid

I don't personally think it's necessary (or even feasible) for this method to provide a comprehensive way of inferring every feature already supported by the Schema class. It seems the scenario @svisser had in mind was the case of enums (["male", "female", "other"]); I am personally fine with a limitation of this method being that you can't infer those, or ranges, or callables, or any number of other things. But it does provide a very easy way of, for example, quickly scaffolding a suite of API regression tests.

@alecthomas
Copy link
Owner

alecthomas commented Nov 24, 2017

I think it's fine to merge this, but if you could add something like the following to the docstring: "Note: only very basic inference is supported".

@dtao
Copy link
Contributor Author

dtao commented Dec 1, 2017

@alecthomas I added the note that you suggested. I thought about being a bit more verbose and explaining more, but hopefully the example in the docstring with the note together will do most of the work to manage expectations.

@coveralls
Copy link

coveralls commented Dec 1, 2017

Coverage Status

Coverage increased (+0.06%) to 95.379% when pulling b710aa0 on dtao:dtao/add-schema-infer-method into 95489bd on alecthomas:master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 1, 2017

Coverage Status

Coverage increased (+0.06%) to 95.379% when pulling b710aa0 on dtao:dtao/add-schema-infer-method into 95489bd on alecthomas:master.

@alecthomas
Copy link
Owner

Yeah, that's great, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants